Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Run tekton containers as nonroot #2435

Closed
wants to merge 1 commit into from
Closed

Run tekton containers as nonroot #2435

wants to merge 1 commit into from

Conversation

mattmoor
Copy link
Member

@mattmoor mattmoor commented Apr 18, 2020 •
edited
Loading

This changes a slew of containers that Tekton runs to use non-root base images.

I was hoping to use Tekton's CI to verify what blows up from this, but I am now realizing I'm not an org member, so someone is going to need to /ok-to-test this.

cc @bobcatfish @dlorenc @imjasonh @vdemeester How do I ascend to such prestige? 🙏

Release notes:

Switch many of the Tekton images (e.g. controllers) to non-root by default.

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 18, 2020
@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 18, 2020
@tekton-robot
Copy link
Collaborator

Hi @mattmoor. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 18, 2020
@danielhelfand
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 18, 2020
@tekton-robot tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 19, 2020
@mattmoor
Copy link
Member Author

I've been playing with nonroot base images, and unfortunately git-init and creds-init seem to need root for how they currently work for SSH auth.

@mattmoor
Copy link
Member Author

This seems clearly related:

I0419 15:21:08.016]         >>> Container step-source-mkdir-kritis-resource-git-2bkdz:
I0419 15:21:08.017]         mkdir: can't create directory '/pvc/create-file-kritis/': Permission denied

@mattmoor
Copy link
Member Author

It looks like the PVCs are created such that only root can interact with them, so that means here the ShellImage must be root.

@mattmoor
Copy link
Member Author

/retest

@mattmoor
Copy link
Member Author

This was buried in there:

I0419 17:34:02.573] ERROR: test build-gcs-targz-7qwxb=SucceededFalse but should be succeededtrue
I0419 17:34:02.573] ERROR: test build-gcs-zip-2msjc=SucceededFalse but should be succeededtrue
I0419 17:34:02.573] ERROR: one or more YAML tests failed

@mattmoor
Copy link
Member Author

Hmm, the new line in the .ko.yaml isn't working for some reason 🤔

ko publish github.com/tektoncd/pipeline/vendor/github.com/GoogleCloudPlatform/cloud-builders/gcs-fetcher/cmd/gcs-fetcher
2020/04/19 11:47:41 Using base gcr.io/distroless/static:nonroot for github.com/tektoncd/pipeline/vendor/github.com/GoogleCloudPlatform/cloud-builders/gcs-fetcher/cmd/gcs-fetcher
2020/04/19 11:47:42 Building github.com/tektoncd/pipeline/vendor/github.com/GoogleCloudPlatform/cloud-builders/gcs-fetcher/cmd/gcs-fetcher
2020/04/19 11:47:49 Publishing gcr.io/mattmoor-knative/gcs-fetcher-029518c065a5d298216f115c6595f133:latest
2020/04/19 11:47:52 Published gcr.io/mattmoor-knative/gcs-fetcher-029518c065a5d298216f115c6595f133@sha256:56b85de03ac8c549649f311fadfe2145c63f4dc825d16b259a31ce88d5dcae5b
gcr.io/mattmoor-knative/gcs-fetcher-029518c065a5d298216f115c6595f133@sha256:56b85de03ac8c549649f311fadfe2145c63f4dc825d16b259a31ce88d5dcae5b

@mattmoor
Copy link
Member Author

Awesome, this is a ko bug. Apparently import paths with uppercase in them are made lowercase when fetched from the viper configuration (.ko.yaml).

From the spf13/viper docs:

Important: Viper configuration keys are case insensitive. There are ongoing discussions about making that optional.

🎉 🤦

mattmoor added a commit to mattmoor/ko that referenced this pull request Apr 19, 2020
@mattmoor
Viper keys are case insensitive.
f915bd3 
This fixes an issue where import paths with uppercase (github.com/GoogleCloudPlatform 👀) were being canonicalized by Viper to all lowercase and the baseImageOverrides in `.ko.yaml` were failing to properly identify the base image.

I hit this in: tektoncd/pipeline#2435 trying to opt some of the GCP images out of `:nonroot`.

This also adds some logging to a place where I see a lot of folks have issues with `ko` where we swallow an error.  I hit it experimenting with variants on the `ko publish` import path, and added the logging to debug.
@mattmoor mattmoor mentioned this pull request Apr 19, 2020
Merged
mattmoor added a commit to ko-build/ko that referenced this pull request Apr 20, 2020
@mattmoor
Viper keys are case insensitive. (#150)
f45bc13 
* Viper keys are case insensitive.

This fixes an issue where import paths with uppercase (github.com/GoogleCloudPlatform 👀) were being canonicalized by Viper to all lowercase and the baseImageOverrides in `.ko.yaml` were failing to properly identify the base image.

I hit this in: tektoncd/pipeline#2435 trying to opt some of the GCP images out of `:nonroot`.

This also adds some logging to a place where I see a lot of folks have issues with `ko` where we swallow an error.  I hit it experimenting with variants on the `ko publish` import path, and added the logging to debug.

* Drop the new log statement
@mattmoor
Copy link
Member Author

The ko change has merged. Who do I talk to about upgrading the version used in CI? @bobcatfish @imjasonh @vdemeester @afrittoli

@vdemeester
Copy link
Member

@mattmoor me 😛

@vdemeester
Copy link
Member

So it should be available on the :latest test-runner image. I'll re-enable those on experimental today and if it all goes well, here too 👼

@mattmoor
Copy link
Member Author

/retest

@mattmoor
Copy link
Member Author

Looks like it hasn't been updated.

@vdemeester
Copy link
Member

Looks like it hasn't been updated.

yep it needs tektoncd/plumbing#352 to be in 👼

@vdemeester
Copy link
Member

/retest

1 similar comment
@vdemeester
Copy link
Member

/retest

@mattmoor
Copy link
Member Author

So this is good now?

afrittoli
afrittoli reviewed Apr 24, 2020
View reviewed changes
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! The code looks change, I look forward to having rootless images were possible,
I think it's worth having release notes for this change, would you mind adding them to the PR description?
/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2020
@mattmoor
Copy link
Member Author

@afrittoli done, lmk if I did it in the way y'all expect.

@afrittoli
Copy link
Member

@afrittoli done, lmk if I did it in the way y'all expect.

Thank you!

1 similar comment
@afrittoli
Copy link
Member

@afrittoli done, lmk if I did it in the way y'all expect.

Thank you!

afrittoli
afrittoli reviewed Apr 29, 2020
View reviewed changes
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2020
@afrittoli
Copy link
Member

afrittoli commented Apr 29, 2020 •
edited
Loading

@mattmoor I'm not sure how to re-trigger the CLA check, would you mind re-pushing this, it should run then. I removed the "lgtm" for now, else this will hold the tide queue

@afrittoli afrittoli removed the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2020
@vdemeester
Copy link
Member

This might need some changes in the publish.yaml task too

@vdemeester
Copy link
Member

ping @mattmoor

@vdemeester vdemeester mentioned this pull request May 12, 2020
Merged
3 tasks
@mattmoor
Copy link
Member Author

I think I'm almost there on approvals, but as discussed in slack, you should totally feel free to take over this work while I sit in limbo :(

@mattmoor mattmoor closed this May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afrittoli afrittoli afrittoli left review comments

@bobcatfish bobcatfish Awaiting requested review from bobcatfish

@imjasonh imjasonh Awaiting requested review from imjasonh

Assignees

@afrittoli afrittoli

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mattmoor @tekton-robot @danielhelfand @vdemeester @afrittoli @googlebot